-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add custom missing mandatory UFO path error types & missing mandatory UFO path checks #146
Add custom missing mandatory UFO path error types & missing mandatory UFO path checks #146
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
I'll wrap up the items in the TODO list and then convert out of Draft for a full implementation review. If this approach seems reasonable, this should get us to read validations / error handling on all required UFO paths |
This PR now includes:
Ready for review. |
src/font.rs
Outdated
let font_load_res = Font::load(path); | ||
match font_load_res { | ||
Ok(_) => panic!("unxpected Ok result"), | ||
Err(Error::MissingUfoDir(_)) => (), // expected value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert_eq!(font_load_res, Err(Error::MissingUfoDir(...)))
or something maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried. Compiler won't let me perform equality comparisons on the Result Error type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two things:
As written, I would do this as,
assert!(matches!(font_load_res, Err(Error::MissingUfoDir(_))));
but there's probably a better option: explicitly testing for a panic with the should_panic
attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but there's probably a better option: explicitly testing for a panic with the should_panic attribute.
Ah nice, I didn't realize that you can add an expected string with should_panic
. I tried this approach but I can't elicit a panic from the new custom Error. The attribute doesn't seem to catch errors like it does panics. I see this in the docs that you linked:
You can’t use the #[should_panic] annotation on tests that use Result<T, E>. Instead, you should return an Err value directly when the test should fail.
Let me tinker with the test Result approach a bit. If I can't find a better solution, I'll transition things over the matches! macro approach above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it would work if you just unwrap
the Result
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(but matches!
is totally reasonable too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the use of a Result return type in unit tests is good if you expect an Ok() but there isn't a way to expect an Err. We could return a test error when the function being tested returns Ok 💥 but that seems worse than the match pattern matching approach :)
I went with the matches! macro. That allows us to confirm the custom error type and works well!
Maybe consider removing inessential files from the test UFOs, like lib.plist. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, feel free to merge at your leisure @chrissimpkins.
Thank you both for all of the feedback here! |
Supersedes #143
New approach based on suggestion in #143 (comment)
TODO: